Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add Sentry screenName tracking #4646

Merged
merged 21 commits into from
Jan 13, 2025
Merged

feat: add Sentry screenName tracking #4646

merged 21 commits into from
Jan 13, 2025

Conversation

m1entus
Copy link
Contributor

@m1entus m1entus commented Dec 18, 2024 •
edited
Loading

📜 Description

Added UIViewController custom screenName tracking ref: #4642

💡 Motivation and Context

I used

protocol SentryViewControllerBreadcrumbTracking {
    var screenName: String { get }
}

If viewController responds to SentryViewControllerBreadcrumbTracking then use screenName instead of [SwiftDescriptor getObjectClassName:controller]

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@codecov Codecov
Copy link

codecov bot commented Dec 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.83051% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.245%. Comparing base (e2484ac) to head (467fd94).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ons/Breadcrumbs/SentryBreadcrumbTrackerTests.swift 88.636% 5 Missing ⚠️
Sources/Sentry/SentryBreadcrumbTracker.m 66.666% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4646       +/-   ##
=============================================
- Coverage   91.251%   91.245%   -0.007%     
=============================================
  Files          624       624               
  Lines        72036     72064       +28     
  Branches     26215     26159       -56     
=============================================
+ Hits         65734     65755       +21     
- Misses        6204      6210        +6     
- Partials        98        99        +1
Files with missing lines Coverage Δ
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryUIApplication.m 57.222% <100.000%> (ø)
.../Sentry/SentryUIViewControllerPerformanceTracker.m 99.267% <100.000%> (ø)
Sources/Sentry/SentryViewHierarchy.m 100.000% <100.000%> (ø)
Sources/Swift/SwiftDescriptor.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryBreadcrumbTracker.m 87.623% <66.666%> (ø)
...ons/Breadcrumbs/SentryBreadcrumbTrackerTests.swift 94.687% <88.636%> (-0.965%) ⬇️

... and 22 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2484ac...467fd94. Read the comment docs.

@m1entus @brustolin
Update SentryViewControllerBreadcrumbTracking.swift
efd328d 
Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>
@m1entus m1entus requested a review from brustolin December 18, 2024 12:56
@kahest kahest linked an issue Dec 18, 2024 that may be closed by this pull request
Allow to use custom UIViewController name when tracking breadcumbs #4642
Closed
brustolin
brustolin approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @m1entus.

LGTM

@m1entus @philipphofmann
Update SentryViewControllerBreadcrumbTracking.swift
441dd86 
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@philipphofmann
Copy link
Member

@m1entus, I'm sorry I haven't gotten to this yet. I still have some catchup to do after the Christmas holidays. I will definitely review this by the end of the week.

@m1entus
Copy link
Contributor Author

m1entus commented Jan 7, 2025

@philipphofmann Cool thanks, no worries - take your time

@m1entus m1entus requested a review from philipphofmann January 9, 2025 14:16
m1entus and others added 2 commits January 9, 2025 15:23
@m1entus @philipphofmann
Update CHANGELOG.md
706cf4c 
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@m1entus
Update ViewController casting
a979c90
philipphofmann
philipphofmann reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update. Almost, LGTM.

Sources/Swift/Protocol/SentryViewControllerBreadcrumbTracking.swift Outdated Show resolved Hide resolved
Sources/Swift/Protocol/SentryViewControllerBreadcrumbTracking.swift Outdated Show resolved Hide resolved
Sources/Swift/SwiftDescriptor.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
m1entus and others added 3 commits January 9, 2025 15:27
@m1entus @philipphofmann
Update Sources/Swift/SwiftDescriptor.swift
a17bffb 
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@m1entus @philipphofmann
Update Sources/Swift/Protocol/SentryViewControllerBreadcrumbTracking.…
7ad6ecb 
…swift

Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@m1entus @philipphofmann
Update Sources/Swift/Protocol/SentryViewControllerBreadcrumbTracking.…
653aeee 
…swift

Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@m1entus m1entus requested a review from philipphofmann January 9, 2025 14:32
philipphofmann
philipphofmann approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor Changelog entry, which won't block the PR. I need to wait for CI to be green and then we can merge this. Thanks @m1entus.

CHANGELOG.md Outdated Show resolved Hide resolved
@m1entus @philipphofmann
Update CHANGELOG.md
1b5705a 
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
@philipphofmann
Copy link
Member

@m1entus, CI is complaining. You can ignore Benchmarking and Build / Release, but the other errors need fixing.

@m1entus
Copy link
Contributor Author

m1entus commented Jan 9, 2025

@m1entus, CI is complaining. You can ignore Benchmarking and Build / Release, but the other errors need fixing.

@philipphofmann Could you tell me what should be fixes? I see that some tests are failing because error cloning repo - so not fully sure what should be fixed ?

@philipphofmann philipphofmann merged commit bfb4bcf into getsentry:main Jan 13, 2025
60 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brustolin brustolin brustolin approved these changes

@philipphofmann philipphofmann philipphofmann approved these changes

@armcknight armcknight Awaiting requested review from armcknight armcknight is a code owner

@philprime philprime Awaiting requested review from philprime philprime is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to use custom UIViewController name when tracking breadcumbs
3 participants
@m1entus @philipphofmann @brustolin